-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Frontend][TensorFlow]TensorFlow Parser Control Flow Enhancement #5020
Conversation
31816bf
to
81f6d17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comment. Need to read it one more time.
cc @gmagogsfm who might be also interested.
def b(i): return [tf.add(i, 1), tf.add(i, 1) + slice] | ||
r = tf.while_loop(c, b, [i]) | ||
|
||
If we directly create recursive function, slice will be placed into function body. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to add a loop-invariant code motion (LICM) pass so that we don't need to handle this in different frameworks. It can benefit general Relay program as well, though it is not necessarily in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In the future we can add a pass automatically detect invariant part inside loop/function, I'll add a TODO here.
"""Find name of direct parent while loop.""" | ||
ploop_name = "" | ||
name_prefix = node_name.rsplit('/', 1)[0] | ||
if name_prefix.startswith("^"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why start with "^"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In certain cases, some nodes under while block will start will ^
sign, but they are still under the same while loop.
name_prefix = node_name.rsplit('/', 1)[0] | ||
if name_prefix.startswith("^"): | ||
name_prefix = name_prefix[1:] | ||
for lname in self._while_loop_name_set: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to explain how the parent and child relationship is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
def visit(self, expr): | ||
""" | ||
For each expression in the body, look up the corresponding | ||
tensorflow node with its structural hash. If the current loop is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TensorFlow
81f6d17
to
5e58143
Compare
Since a tensorflow op |
5e58143
to
89cbe41
Compare
@zhiics PTAL |
@kevinthesun sorry, will do by tomorrow, too much stuff recently. |
449138c
to
97ba277
Compare
@srkreddy1238 Could you also take a look? |
f987edd
to
3a1130f
Compare
3a1130f
to
f8de9a7
Compare
Thanks @kevinthesun |
…che#5020) * Improve TF control flow major logic * Pass mod into operator convert function * Fix LoopBound * Add more control flow tests * Add two test cases for stridedslice * Fix docstring * Fix lint * Fix import * Fix test assert * Minor fix conv3d * Add more comments * Fix for dilation2d * Change newly added atan * Change newly added unravel
…che#5020) * Improve TF control flow major logic * Pass mod into operator convert function * Fix LoopBound * Add more control flow tests * Add two test cases for stridedslice * Fix docstring * Fix lint * Fix import * Fix test assert * Minor fix conv3d * Add more comments * Fix for dilation2d * Change newly added atan * Change newly added unravel
RFC: #4969
In addition to control flow improvements, this PR also changes some operator parsing logic to support symbolic input for certain attributes.
@zhiics @yongwww @jroesch @srkreddy1238 @masahi